C++: Abstractions for treating qualifiers as parameters in IR#4702
Conversation
MathiasVP
changed the title
|
CPP-difference started: https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1563/ |
|
I think we want to guide users towards using these predicates by default, which usually means giving them names that are simpler than their alternatives. Could |
…uide users to use these predicates by default.
… own implementation of getArgument already (which didn't handle qualifiers). The predicate wasn't used anywhere, so I simply removed it, as a better predicate is now available on the base class of DataFlowCall.
I think those are some excellent names! I've changed the names to match your suggestions now. I had to get rid of an unused predicate on DataFlowCall in |
CPP-difference looks good (for some reason there's a 10% reduction in build/extraction/import time on openjdk?) |
jbj
left a comment
jbj
left a comment
There was a problem hiding this comment.
Looks great! I'm happy with the names too.
| final predicate hasIndex(int index) { | ||
| index >= 0 and index = this.getParameter().getIndex() | ||
| or | ||
| index = -1 and this.isThisIndirection() | ||
| } |
There was a problem hiding this comment.
The implementation of this predicate looks a lot like getArgumentOperand, but only the latter has pragma[noinline] on it. Is there a good reason to put pragma[noinline] on one but not the other?
There was a problem hiding this comment.
Hm, no. I suspect the pragma[noinline] is an artifact of me copy/pasting from getPositionalArgumentOperand. I'll just double-check that I can safely remove the new pragma[noinline]s.
There was a problem hiding this comment.
Hm... looking at this comment you made, I'd actually feel more comfortable adding a pragma[noinline] to the hasIndex predicates. It doesn't seem to be necessary right now, but if it avoids a future bad join-order we might as well put it there now. Would that be reasonable?
3 participants
Fixes https://github.com/github/codeql-c-analysis-team/issues/186.
In #3571 we made it possible to treat
thisas an argument, but there are still a couple of places where we need to distinguish between qualifiers and parameters (and betweenthisand arguments). These places are:CallInstruction(which has agetPositionalArgumentandgetThisArgumentpredicates)getParameterpredicate, but instructions initializingthisneed to refer to theIRVariable). The same problem also exists forReturnIndirectionInstruction.This PR adds a new predicate for each of these scenarios:
CallInstructionnow has agetPositionalOrThisArgumentOperand(andgetPositionalOrThisArgument) predicates that, given an index, gives back the argumentoperand(andinstruction) with the given index, or when the index is-1, gives back the operand used as the qualifier.InitializeParameterInstructionandInitializeIndirectionInstructionnow have anisParameterOrQualifierIndexpredicate that holds if the instruction initializes a positional parameter with a given index, or initializethiswith the index-1.Similarly,
ReturnIndirectionInstructionhas a new predicateisParameterOrThisIndirectionwith similar semantics.